-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add a devtool for looking at users and their devices #30983
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
ed491d7
to
3a55af8
Compare
fc12768
to
26cb5cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, looks very nice. A few stylistic things.
* | ||
* By default, filters to only show joined users. | ||
* | ||
* If the `member` state is set, delegates to `User` to view a single user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is User
? Should this be an {@link}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also: the talk of state seems like an implementation detail which does not belong in the doc-comment?
* If the `member` state is set, delegates to `User` to view a single user. | |
* Once the user chooses a specific member, delegates to {@link UserView} to view a single user. |
/** | ||
* Shows a list of users in the room, and allows selecting a user to view. | ||
* | ||
* By default, filters to only show joined users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does one override this default, as a user of the component?
/** | ||
* Shows a single user to view, and allows selecting a device to view. | ||
* | ||
* If the `device` state is set, delegates to `Device` to show a single device. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above, this needs attention
const UserView: React.FC<UserProps> = ({ member, onBack }) => { | ||
const context = useContext(DevtoolsContext); | ||
const crypto = context.room.client.getCrypto(); | ||
const verificationStatus = useAsyncMemo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would probably be helpful to add a comment documenting the type and possible values of verificationStatus
, since it's not obvious at a glance
hideTooltip={true} | ||
status={E2EStatus.Verified} | ||
className="mx_E2EIcon_inline" | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm no react expert, but ... putting this into useAsyncMemo
feels a bit cumbersome to me. Wouldn't it be more natural to just to do:
const verificationStatus = useAsyncMemo(async () => {
return await crypto?.getUserVerificationStatus(member.userId);
}, ... );
... and put the rest outside useAsyncMemo
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually how I had it originally, but it all migrated into the useAsyncMemo
. I don't remember the exact reasons. I think part of it was you'd end up checking if crypto
is available twice (once with the crypto?
inside the useAsyncMemo
, and the once with an if verificationStatus == null
outside). Another reason was that this way I could make it a const
whereas if I put the rest outside, I'd need to create a variable, and then set the value in an if
statement. Those are both pretty small reasons, so if the other way is a better match to the coding style of the rest of the app, then I can change it.
const devices = await crypto?.getUserDeviceInfo([member.userId]); | ||
return devices?.get(member.userId) ?? new Map(); | ||
}, | ||
[context], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[context], | |
[context, member], |
const _onBack = (): void => { | ||
setMember(null); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be inclined to inline this into the <UserView>
invocation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was copying from another file in the directory, but yeah, it's short enough that it can be inlined.
"unverified": "Verification status: <E2EIcon /> Not signed by owner", | ||
"verified": "Verification status: <E2EIcon /> Verified by cross-signing" | ||
}, | ||
"devices": "Devices (%(count)s)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably say something to remind us that this is only devices with keys
"devices": "Devices (%(count)s)", | |
"devices": "Cryptographic devices (%(count)s)", |
maybe? dunno
); | ||
} | ||
}, | ||
[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[], | |
[crypto, device], |
return _t("devtools|device_verification_status|unverified", {}, { E2EIcon: e2eIcon }); | ||
} | ||
}, | ||
[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[], | |
[crypto, device], |
Fixes #30946
Adds a devtool that allows you to look at users and their devices, and shows information about them, including verification status.
Checklist
public
/exported
symbols have accurate TSDoc documentation.